Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show paths relative to the source root for diagnostics in sub-workspaces #128904

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Aug 9, 2024

This allows rust-analyzer to show inline errors for the library workspace. Clippy recently started doing something similar.

Fixes #128726 which is a regression from #128534

This allows rust-analyzer to show inline errors for the library
workspace. Clippy recently started doing something similar.
@rustbot
Copy link
Collaborator

rustbot commented Aug 9, 2024

r? @onur-ozkan

rustbot has assigned @onur-ozkan.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Aug 9, 2024
if let Ok(prefix) = env::current_dir().unwrap().strip_prefix(&rustc_source_dir) {
prefix.to_owned()
} else {
PathBuf::from(".")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does a =. remap even do? Wouldn't it make more sense to only add --remap-path-prefix if strip_prefix succeeds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a leftover from a different attempt at fixing this issue. It should indeed skip --remap-path-prefix in this case.

@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2024

This is a neat hack!

I wonder, does it fundamentally rely on running inside the rustc wrapper? Or is there a way to do this that will work even for a regular cargo invocation? Clippy and Miri don't have any workspaces themselves so this hack should work just fine for them, but if a project had more than one workspace and a wrapper script then is it possible to get the right remapping for all crates inside that workspace?

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 9, 2024

I wonder, does it fundamentally rely on running inside the rustc wrapper? Or is there a way to do this that will work even for a regular cargo invocation?

I think it fundamentally needs the wrapper to ensure that the remapping only applies to workspace members and not deps from crates.io, which have a different cwd. Also without the wrapper, you did have to be careful to pass the right --remap-path-prefix for each cargo invocation, while with the wrapper the right value is automatically calculated.

but if a project had more than one workspace and a wrapper script then is it possible to get the right remapping for all crates inside that workspace?

If the project has it's own RUSTC_WRAPPER, then this wrapper needs to copy the code from bootstrap's rustc wrapper.

@RalfJung
Copy link
Member

RalfJung commented Aug 9, 2024

If the project has it's own RUSTC_WRAPPER, then this wrapper needs to copy the code from bootstrap's rustc wrapper.

Sorry, I meant a wrapper script like ./miri. There we now set --remap-path-prefix before invoking cargo. I wonder what that would do if we had a workspace with crates in different directories... but maybe that's too hypothetical.

@onur-ozkan onur-ozkan added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 11, 2024
@bors
Copy link
Contributor

bors commented Aug 11, 2024

☔ The latest upstream changes (presumably #122362) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

What's the plan for this PR? This lack diagnostics in library/ is starting to be somewhat annoying.

It seems like rust-lang/cargo#13644 will provide an alternative to using path remapping?

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 24, 2024

--remap-path-prefix also affects the emitted crate metadata, which breaks the remapping for the standard library source to the rust-src component done by rustc when the rust-src component is available. A diagnostics-only flag would be necessary for this approach to work.

rust-lang/cargo#13644 doesn't seem to be related at all. It only adds an env var that can be read at compile time. It doesn't provide a way to actually change the working directory for spawned rustc processes.

@RalfJung
Copy link
Member

RalfJung commented Aug 24, 2024 via email

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 24, 2024

The actual code of that PR only sets an env var. It doesn't read one. It provides the working directory of rustc to the to be compiled code as env var.

@RalfJung
Copy link
Member

Oh I see, I entirely misunderstood the point of that PR then, sorry.

--remap-path-prefix also affects the emitted crate metadata, which breaks the remapping for the standard library source to the rust-src component done by rustc when the rust-src component is available. A diagnostics-only flag would be necessary for this approach to work.

So this is a problem even if the flag is only applied during development, not for any distributed artifacts?

If --remap-path-prefix is not an option I guess it's back to this proposal?

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 25, 2024

So this is a problem even if the flag is only applied during development, not for any distributed artifacts?

Yes, it causes several tests to fail due to differing diagnostics as can be seen in the failing CI run.

@Alexendoo
Copy link
Member

If there aren't any other remaps that need the other scopes -Zremap-path-scope=diagnostics could work

@bjorn3
Copy link
Member Author

bjorn3 commented Aug 25, 2024

There are other --remap-path-prefix args when setting rust.remap-debuginfo in config.toml.

@RalfJung
Copy link
Member

Sounds like it'd be good to set the remap scope not globally but separately for each remapped path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust-analyzer + rustc repo: build failures in the standard library are not shown in the editor any more
6 participants